Skip to content

feat(2fa): trusted devices management UI and revoke endpoints - #160

Open
romanetar wants to merge 2 commits into
feat/mfa-phase1---migrations--and--interfacesfrom
feat/mfa-trusted-devices-management
Open

romanetar wants to merge 2 commits into
feat/mfa-phase1---migrations--and--interfacesfrom
feat/mfa-trusted-devices-management

Conversation

@romanetar

@romanetar romanetar commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

ref https://app.clickup.com/t/86bc647cd (SDS idp-mfa.md 5.5 / 5.6)

Summary

Users could mark a device as trusted when completing a 2FA challenge, but had no way to see or revoke those devices: a lost or shared device kept bypassing the challenge until the cookie expired (30 days by default). This adds self-service listing and revocation, on the API and on the profile page.

Backend

New routes in the existing admin/api/v1 → users/me group (session-authenticated, ssl + auth):

Method Route Response
GET /admin/api/v1/users/me/2fa/devices 200 {data: [...]} with the caller's active devices
DELETE /admin/api/v1/users/me/2fa/devices/{id} 204 (csrf)
DELETE /admin/api/v1/users/me/2fa/devices 204 (csrf)
  • Listing: only non-revoked, non-expired devices of the caller. UserTrustedDeviceSerializer returns id, device_name, ip_address, trusted_at, expires_at, last_seen_at and is_current (the device whose cookie came with the request). It never exposes device_identifier or user_agent, and it reads through the repository, so it does not refresh last_seen_at the way isDeviceTrusted() does.
  • Ownership: IUserTrustedDeviceRepository::getByIdAndUser() scopes the lookup by owner. Another user's device id is indistinguishable from a missing one (404), and the row is left untouched.
  • Audit semantics (agreed on the ticket): device_revoked is logged only on a real false → true transition of is_revoked, one row per device.
    • Revoking an already revoked or expired device is a 204 no-op with no audit row.
    • Revoke-all logs one event per device it actually revokes, and none when there are no active devices.
  • Cookie: when the revoked device is the current one, an expired device-trust cookie is queued with the same name, path, domain and flags as queueDeviceTrustCookie() (MFACookieManager::expireDeviceTrustCookie()), so the next login is challenged. The session itself stays active.
  • removeTrustedDevices() now revokes active devices entity by entity inside a transaction and returns them. The old bulk revokeAllForUser() DQL UPDATE is removed; its only caller was this method, and it could not tell which devices it revoked.

Frontend

  • New TrustedDevicesSection (resources/js/components/trusted_devices_section.js), mounted under the 2FA section of the profile page and shown only when twoFactorEnabled:
    • table with device, IP, trusted, last seen and expiry, plus a "This device" label;
    • a Revoke button per row that removes the row in place;
    • "Revoke all devices" behind a Swal confirmation, with a note that the current session stays active;
    • empty state when there are no devices.
  • Buttons are plain onClick (the profile page is a single <form>).
  • Endpoints exposed on window from profile.blade.php; fetch helpers in resources/js/profile/actions.js.

Deviations from the SDS / ticket

  • Controller: the endpoints live in Api\UserApiController under users/me/2fa in routes/web.php, next to 2fa/enable and recovery-codes/regenerate, instead of the SDS's new Api\UserTwoFactorApiController in routes/api.php. This avoids splitting the same resource across two controllers.
  • Unauthenticated calls return 302 to /auth/login, not 403: the group's auth middleware rejects guests before the controller runs, same as the sibling routes. The ticket's acceptance criterion was updated accordingly.
  • Audit is written after the revocation commits (best-effort), not in the same transaction: ITwoFactorAuditService::log() opens its own transaction, and nesting DoctrineTransactionService::transaction() calls is unsafe (see RecoveryCodeService::enableTwoFactorAndGenerateCodes()). This follows the same pattern: an audit failure is logged as a warning and does not fail an already-committed revocation.
  • Session flow key: the ticket's criterion says the next login lands on flow = mfa; the actual session value is 2fa, and that is what the test asserts.
  • The SDS names the cookie fn_device_trust; the code keeps reading config('two_factor.cookie_name').

Testing

  • tests/TrustedDevicesApiTest.php (new, 15 tests):
    • listing scope, field exposure, is_current and last_seen_at untouched;
    • single revoke with audit, plus the no-op revoke for revoked and for expired devices;
    • cross-user 404 and unknown id 404;
    • revoke-all with one audit row per device and zero when there are none;
    • cookie expired only for the current device;
    • a revoked device's old token is challenged again on password login;
    • unauthenticated 302 on all three routes.
  • tests/DeviceTrustServiceTest.php: updated for the new removeTrustedDevices() behaviour; new unit tests for revokeTrustedDevice() and for surviving an audit failure.
  • tests/TwoFactorProfileRoutesCsrfTest.php: both DELETE routes added.
  • Jest: tests/js/components/trusted_devices_section.test.js (render, current-device label, empty state, revoke, failed revoke, revoke-all confirm/cancel, no submit buttons) and tests/js/profile/actions.test.js (the three helpers against the unmocked request layer).

Results:

  • PHPUnit (MFA-related files only): all green, 16 files / 178 tests. DeviceTrustServiceTest, TrustedDevicesApiTest, TwoFactorProfileRoutesCsrfTest, TwoFactorRepositoriesTest, TwoFactorLoginFlowTest, RecoveryCodeRegenerationTest, OAuth2NativeMFALoginFlowTest, VerifyOTPChallengeTest, AuthServiceLoginUserTest and the tests/unit MFA / 2FA tests. The full PHPUnit suite was not run.
  • Jest: yarn test:unit green, 13 suites / 76 tests.
  • Webpack: production build compiles.
  • The UI was not exercised manually in a browser.

Out of scope

2FA disable toggle and method selection, phone verification, admin-side device management, and friendly user-agent parsing for device_name.

Summary by CodeRabbit

  • New Features
    • Added a Trusted Devices section to profiles with two-factor authentication, showing device details and identifying the current device.
    • Users can revoke a single trusted device or all trusted devices. Revoking devices leaves the current session active; revoked devices must complete verification at their next login.
  • Bug Fixes
    • Revoking the current device clears its trusted-device status in the browser.
  • Tests
    • Added coverage for device listings, revocation, login verification, and error handling.

Add GET/DELETE /admin/api/v1/users/me/2fa/devices[/{id}] so a user can
list their active trusted devices and revoke one or all of them
(SDS idp-mfa.md 5.6, CU-86bc647cd).

- Revocation is scoped by owner: another user's device id returns 404.
- Revoking an already revoked or expired device is a 204 no-op and is
  not audited; revoke-all logs one device_revoked event per device it
  actually revokes and none when there are no active devices.
- Audit runs best-effort after the revocation commits, following
  RecoveryCodeService: audit_service->log() opens its own transaction,
  which cannot be nested.
- The device-trust cookie is expired when the revoked device is the one
  the request came from.
- The list never exposes device_identifier or user_agent and flags the
  current device via is_current; it does not touch last_seen_at.
- Replace the bulk revokeAllForUser() UPDATE with a per-entity revoke of
  active devices so each revoked device can be audited.
Add a Trusted Devices subsection to the profile page (SDS idp-mfa.md
5.5, CU-86bc647cd), shown only when 2FA applies to the user. It lists
the user's active trusted devices (name, IP, trusted, last seen,
expiry), labels the current one, and lets the user revoke a single
device or all of them (with a confirmation), updating the table in
place. Buttons are plain onClick handlers since the profile page is a
single <form>.

Expose the three device endpoints on window from profile.blade.php and
add getTrustedDevices / revokeTrustedDevice / revokeAllTrustedDevices
helpers, covered by Jest tests for the component and the unmocked
request layer.
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: ab057271-6d7b-42ff-b8a4-4e63e06a5ce1

📥 Commits

Reviewing files that changed from the base of the PR and between 2d489c3 and e154749.

📒 Files selected for processing (18)
  • app/Http/Controllers/Api/UserApiController.php
  • app/Http/Controllers/Traits/MFACookieManager.php
  • app/ModelSerializers/Auth/UserTrustedDeviceSerializer.php
  • app/ModelSerializers/SerializerRegistry.php
  • app/Repositories/DoctrineUserTrustedDeviceRepository.php
  • app/Services/Auth/DeviceTrustService.php
  • app/Services/Auth/IDeviceTrustService.php
  • app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php
  • resources/js/components/trusted_devices_section.js
  • resources/js/profile/actions.js
  • resources/js/profile/profile.js
  • resources/views/profile.blade.php
  • routes/web.php
  • tests/DeviceTrustServiceTest.php
  • tests/TrustedDevicesApiTest.php
  • tests/TwoFactorProfileRoutesCsrfTest.php
  • tests/js/components/trusted_devices_section.test.js
  • tests/js/profile/actions.test.js

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change adds authenticated endpoints to list and revoke trusted devices. It adds service-level ownership checks and revocation auditing, then exposes device status and revocation controls on the profile page when two-factor authentication is enabled.

Changes

Trusted device management

Layer / File(s) Summary
Device retrieval and revocation
app/libs/Auth/Repositories/IUserTrustedDeviceRepository.php, app/Repositories/DoctrineUserTrustedDeviceRepository.php, app/Services/Auth/IDeviceTrustService.php, app/Services/Auth/DeviceTrustService.php, tests/DeviceTrustServiceTest.php
The service retrieves active devices and revokes devices scoped to their owner. Successful revocations are audited individually; audit failures are logged. Tests cover bulk and individual revocation, ownership, idempotency, and audit failures.
Trusted-device API and serialization
app/ModelSerializers/Auth/UserTrustedDeviceSerializer.php, app/ModelSerializers/SerializerRegistry.php, app/Http/Controllers/Api/UserApiController.php, app/Http/Controllers/Traits/MFACookieManager.php, routes/web.php, tests/TrustedDevicesApiTest.php, tests/TwoFactorProfileRoutesCsrfTest.php
Authenticated routes list active devices and revoke one or all devices. Responses identify the current device without exposing its identifier or raw user agent. Revocation expires the trust cookie when the current device is among those revoked.
Profile controls and client actions
resources/js/profile/actions.js, resources/views/profile.blade.php, resources/js/profile/profile.js, resources/js/components/trusted_devices_section.js, tests/js/profile/actions.test.js, tests/js/components/trusted_devices_section.test.js
The profile displays trusted-device details and revocation controls when two-factor authentication is enabled. Client actions call the list and revocation endpoints. Tests cover loading, display, confirmation, and revocation results.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant TrustedDevicesSection
  participant UserApiController
  participant DeviceTrustService
  participant TrustedDeviceRepository
  participant AuditLogger
  TrustedDevicesSection->>UserApiController: Request device list or revocation
  UserApiController->>DeviceTrustService: Retrieve or revoke devices for authenticated user
  DeviceTrustService->>TrustedDeviceRepository: Read active devices or owner-scoped device
  TrustedDeviceRepository-->>DeviceTrustService: Return matching device records
  DeviceTrustService->>TrustedDeviceRepository: Persist device revocations
  DeviceTrustService->>AuditLogger: Attempt per-device revocation audit
  DeviceTrustService-->>UserApiController: Return devices or revocation result
  UserApiController-->>TrustedDevicesSection: Return serialized devices or deleted response
Loading

Suggested reviewers: matiasperrone-exo, smarcet

Merge Risk: ⚪ Minimal · up to e1547

The reviewed trusted-device listing and revocation paths have no established issue requiring a fix before merge. The reported end-to-end test failure was not independently established as a regression from this change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 18 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: trusted-device management UI and revocation endpoints for 2FA.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-160/

This page is automatically updated on each push to this PR.

@romanetar

Copy link
Copy Markdown
Contributor Author

@smarcet the e2e job on this PR fails, but the failure is not caused by this PR. It comes from the base branch: feat/mfa-phase1---migrations--and--interfaces e2e runs have been failing since 9f24344d chore: fix ssl redirect.

Symptom: every login e2e test (login-mfa-flow.spec.ts, login.spec.ts, auth-code-flow.spec.ts) waits for #email until the 30s timeout. The last green e2e run on the base branch was 807befd2 (#151), right before that commit. It doesn't show up on main because main has no e2e workflow.

Root cause (from the Playwright trace artifact): the page is served from http://localhost:8001/auth/login, but its assets are requested from https://localhost:8001/assets/login.js and .../css/login.css. CI runs the app with php artisan serve, which is plain HTTP, so those requests fail with ERR_CONNECTION_CLOSED, the bundle never loads and the page stays blank.

9f24344d switched AppServiceProvider::boot() back to the condition main uses:

-        if (Config::get('server.ssl_enabled', false))
+        if (!App::isLocal())
             URL::forceScheme('https');

I understand the ssl_enabled condition from #126 broke the redirect in production (no SSL_ENABLED behind the load balancer). The e2e workflow, however, runs with APP_ENV=testing and SSL_ENABLED=false, so it now forces https URLs against a server without TLS.

Some options that would work for both:

  1. An explicit flag, e.g. FORCE_HTTPS (default true, so production doesn't change), which the e2e workflow sets to false.
  2. Force https unless the app is local or SSL_ENABLED is explicitly set to false in the environment.

Which one do you prefer? I can open a PR against the base branch.

The other checks on this PR are green, including unit-tests (full PHPUnit suite) and js-unit-tests.

@romanetar
romanetar requested a review from smarcet September 28, 2026 15:19

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant